Fix NPE from map corruption when sync handlers register entries during init#128
Fix NPE from map corruption when sync handlers register entries during init#128Luca-Guettinger wants to merge 1 commit into
Conversation
Luca-Guettinger
changed the title
YannickMG
left a comment
YannickMG
left a comment
There was a problem hiding this comment.
I would rather have us catch people trying to register Sync Handlers during sync handler initialization and throw at that time, so the contents of syncHandlers can be relied upon. There isn't actually a scenario where we should be doing this, so we shouldn't stop this from crashing.
If we do end up deciding this is a good idea anyway, at least consider adding some loud logging to the branch where there was a null in the list.
|
I agree with Yannick. We should rather lock before init all sycn handlers. |
|
Also this pr doesnt register the sync handlers that were just registered in other sync handlers init. It only registers the current sync handlers. So you would need put this in a while loop and check when the size no longer changes. |
YannickMG
left a comment
YannickMG
left a comment
There was a problem hiding this comment.
As per discussion, we shouldn't allow the anti pattern of registering sync handlers during sync handler initialisation.
I still suggest that instead we make it crash earlier and/or with a clear debug message.
it does, putSyncValue already handles this recursively. I do get that we dont wannt to discourage this, but i think just adding a log message might be better then restricting it completly, or why would it be bad if we sometimes allow that? |
I dont see where putSyncValue does anything recursively. |
3 participants
PanelSyncManager.initialize() iterates over syncHandlers (an Object2ReferenceLinkedOpenHashMap) via forEach while calling init() on each handler. If a handler registers additional sync handlers during init() (e.g., GT5's GhostCircuitSyncHandler registering an IntSyncValue), the map is structurally modified during iteration. Unlike standard Java collections, fastutil's open-addressing maps don't throw ConcurrentModificationException, they fail silently, producing phantom null entries that cause an NPE. The fix snapshots the map's keys before iterating and looks up each value from the live map. Keys are immutable Strings, so the snapshot is always valid, and get() works correctly after any rehash. Entry objects can't be snapshotted because fastutil's MapEntry stores an internal array index that becomes stale after rehash.